🌱 Rename ClusterObjectSet secret cucumber steps for clarity#2622
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Renames ClusterObjectSet “ref Secrets/refs” cucumber steps to clearer “referred secrets” terminology and factors label-matching logic into a shared helper.
Changes:
- Renamed several ClusterObjectSet e2e step definitions and updated the install feature accordingly.
- Updated the “labels” step to accept a data table for expected labels.
- Added a shared
matchLabelshelper and reused it inResourceHasLabels.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/e2e/steps/steps.go | Renames step functions/patterns, adds matchLabels, and updates label-verification logic to use a data table + helper. |
| test/e2e/features/install.feature | Updates scenario steps to the new “referred secrets” wording and supplies label expectations via a data table. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // matchLabels checks that actual labels contain all expected key-value pairs. | ||
| // Returns the first mismatched key, the actual value, and false if a mismatch is found. | ||
| func matchLabels(actual, expected map[string]string) (key, got string, ok bool) { | ||
| for k, v := range expected { | ||
| if a, found := actual[k]; !found || a != v { | ||
| return k, a, false | ||
| } | ||
| } | ||
| return "", "", true | ||
| } |
There was a problem hiding this comment.
The helper’s “first mismatched key” is nondeterministic because Go map iteration order is random, which can make logs/flakes harder to reproduce. Consider iterating over a stable ordering of keys (e.g., sort the expected keys before checking) so failures are deterministic and easier to debug.
| waitFor(ctx, func() bool { | ||
| secrets, err := listReferredSecrets(ctx, revisionName) | ||
| if err != nil || len(secrets) == 0 { | ||
| return false | ||
| } | ||
| ownerLabel := s.Labels["olm.operatorframework.io/owner-name"] | ||
| if expectedOwner != "" && ownerLabel != expectedOwner { | ||
| return fmt.Errorf("secret %s/%s has owner-name label %q, expected %q", s.Namespace, s.Name, ownerLabel, expectedOwner) | ||
| for _, s := range secrets { | ||
| if _, _, ok := matchLabels(s.Labels, expected); !ok { | ||
| return false | ||
| } | ||
| } | ||
| } | ||
| return true | ||
| }) |
There was a problem hiding this comment.
When label matching fails, the retry loop returns false without emitting any diagnostic signal about which secret failed or which key/value mismatched. Since waitFor will eventually fail with a generic timeout, debugging can be unnecessarily difficult; consider logging the secret name/namespace and the mismatched label key (and expected vs got) on mismatch (similar to the ResourceHasLabels logging).
camilamacedo86
left a comment
camilamacedo86
left a comment
There was a problem hiding this comment.
/approve
I am ok with this one.
We might just want to ensure that we address the nit comments from Copilot
All great !!
openshift-ci
bot
added
the
approved
Rename "ref Secrets/refs" step definitions to "referred secrets" for better readability. Update the labels step to accept a data table and extract a shared matchLabels helper with deterministic key ordering. Add diagnostic logging when label matching fails during polling. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
pedjak
force-pushed
the
rename-cos-cucumber-steps
branch
from
ab09ae3 to
38c03ac
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2622 +/- ##
==========================================
+ Coverage 68.86% 68.88% +0.02%
==========================================
Files 139 139
Lines 9902 9902
==========================================
+ Hits 6819 6821 +2
+ Misses 2572 2571 -1
+ Partials 511 510 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
camilamacedo86
left a comment
camilamacedo86
left a comment
There was a problem hiding this comment.
We are just renaming things
I think it is totally fine to move forward.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 735b41e
into
operator-framework:main
Description
Rename ClusterObjectSet e2e cucumber steps from internal terminology ("refs", "ref Secrets") to more descriptive names and extract a shared
matchLabelshelper.Step renames
phase objects use refsphase objects are managed in Kubernetes secretsref Secrets exist in "..." namespacereferred secrets exist in "..." namespaceref Secrets are immutablereferred secrets are immutableref Secrets are labeled with revision and ownerreferred secrets contain labelsref Secrets have ownerReference to the revisionreferred secrets are owned by the object setref Secrets have type "..."referred secrets have type "..."The "contain labels" step now uses a data table (like
ResourceHasLabels) instead of hardcoded label keys, and both steps share amatchLabelshelper.Reviewer Checklist